Add traefik_route to serve tracing endpoints through ingress if it exists #94

mmkay · 2024-04-25T15:16:00Z

Issue

Tempo receiver endpoints were not accessible over traefik as they are bound to different ports. Fixes #82

Solution

Use traefik_route with changes introduced in traefik-k8s-operator#325 to define ports traefik needs to open.
Also fix creating URLs for tracing consumers that use external_url - scheme and port were missing from the created url.

Context

This change is needed for cross-model traces to work. They cannot access internal endpoints of a different k8s cluster . No traces were also coming from machine models.

Testing Instructions

Run the following bundle and verify endpoints (example, port 3200, /metrics, port 4318 (expected prometheus metrics), /v1/traces (expected HTTP code 405)) are accessible on traefik's external IP.

bundle: kubernetes
applications:
  tempo:
    charm: local:tempo-k8s-1
    scale: 1
    constraints: arch=amd64
    storage:
      data: kubernetes,1,1024M
  traefik-k8s:
    charm: traefik-k8s
    channel: latest/edge
    revision: 184
    base: [email protected]/stable
    resources:
      traefik-image: 159
    scale: 1
    constraints: arch=amd64
    storage:
      configurations: kubernetes,1,1024M
relations:
- - tempo:tracing
  - traefik-k8s:tracing
- - traefik-k8s:traefik-route
  - tempo:ingress

Release Notes

PietroPasotti

Looks good! However I'd really like to see some tests.
At least a couple of unittests for databag contents and endpoints based on whether there is an ingress relation.

Secondly, wondering if we should be renaming the 'ingress' endpoint to traefik-route to follow the convention of having endpoints and interfaces similarly named.
Also, if some day we choose to provide this kind of multi-route, hybrid ingress via traefik and upgrade the ingress relation accordingly, we can swap them out without the ingress name being taken by something that isn't quite ingress.

src/charm.py

charmcraft.yaml

…_relation_created

src/charm.py

mmkay · 2024-04-30T21:23:56Z

@PietroPasotti: regarding this part:

Secondly, wondering if we should be renaming the 'ingress' endpoint to traefik-route to follow the convention of having endpoints and interfaces similarly named.
Also, if some day we choose to provide this kind of multi-route, hybrid ingress via traefik and upgrade the ingress relation accordingly, we can swap them out without the ingress name being taken by something that isn't quite ingress.

I don't mind changing the interface name but I was basing the current approach on that Grafana also uses ingress in the same manner:

https://github.com/canonical/grafana-k8s-operator/blob/main/charmcraft.yaml#L54

As it was the only o11y charm that uses traefik_route that I know of, I chose the same pattern as it is fairly transparent to the charm users - they want to relate to Traefik and they're (mostly) used that the protocol they would use is named "ingress". I can however change that if you think having ingress free for future use is useful :)

PietroPasotti

looking good! a few things that we need to check :)

tests/scenario/test_ingressed_tracing.py

tests/scenario/test_tracing_requirer.py

src/charm.py

PietroPasotti

Mainly the provider endpoint api needs a final touch IMHO

charmcraft.yaml

src/charm.py

PietroPasotti

one final typing suggestion, then we can go!
Only concern I'm left with is the situation where Traefik has TLS, tempo doesn't: external url will be https, internal one will be http.

Will ingestion work via both?

Once you're convinced this is fine, we're good :)

lib/charms/tempo_k8s/v2/tracing.py

Co-authored-by: PietroPasotti <[email protected]>

PietroPasotti

there's something wrong with http/https and ingress after all

lib/charms/tempo_k8s/v2/tracing.py

PietroPasotti

what an emotional rollercoaster

mmkay force-pushed the traefik-route branch from 130af94 to 72ccc51 Compare April 25, 2024 15:16

github-actions bot added the Libraries: Out of sync label Apr 25, 2024

mmkay mentioned this pull request Apr 25, 2024

[WIP] Add nginx #84

Closed

mmkay changed the title ~~[WIP] Add traefik_route to serve tracing endpoints through ingress if it exists~~ Add traefik_route to serve tracing endpoints through ingress if it exists Apr 26, 2024

mmkay marked this pull request as ready for review April 26, 2024 09:08

mmkay requested review from Abuelodelanada, dstathis, IbraAoad, lucabello, PietroPasotti, sed-i and simskij as code owners April 26, 2024 09:08

mmkay requested a review from michaeldmitry April 26, 2024 09:08

PietroPasotti suggested changes Apr 26, 2024

View reviewed changes

mmkay added 8 commits April 29, 2024 14:25

WIP: Use traefik_route to allow other ports through traefik

8264af5

Add http:// to HTTP tracing endpoints behind external_url

70fdede

Send static config entrypoints to traefik_route

f36bdf0

Add port to tracing URL when external_url is used

e8b7455

tox -e fmt

65925f7

linter behaves differently on ci vs locally

81fb018

Generate Traefik config based on Tempo.all_ports

6e38044

-service was missing from HTTP services definition

99adf05

mmkay force-pushed the traefik-route branch from b0d9015 to 99adf05 Compare April 29, 2024 13:07

mmkay added 5 commits April 29, 2024 22:59

wip: Add unit tests

b0c12ef

Unit & scenario tests

8e9955a

Review remarks

21c002d

different formatter again

a872c37

Don't update tracing relations in ingress_relation_joined and ingress…

4f97ae8

…_relation_created

sed-i reviewed Apr 30, 2024

View reviewed changes

src/charm.py Show resolved Hide resolved

src/charm.py Show resolved Hide resolved

src/charm.py Show resolved Hide resolved

mmkay requested a review from PietroPasotti April 30, 2024 21:24

Merge branch 'main' into traefik-route

9e57659

PietroPasotti suggested changes May 2, 2024

View reviewed changes

mmkay added 2 commits May 2, 2024 14:49

Review remarks

60bea29

Remove Harness leftover setting relation without need

656ce73

PietroPasotti suggested changes May 3, 2024

View reviewed changes

charmcraft.yaml Outdated Show resolved Hide resolved

src/charm.py Outdated Show resolved Hide resolved

mmkay added 2 commits May 3, 2024 11:34

Add internal_scheme field exposing internal communication protocol

a25876f

Merge branch 'main' into traefik-route

0f3e3fc

PietroPasotti approved these changes May 3, 2024

View reviewed changes

lib/charms/tempo_k8s/v2/tracing.py Outdated Show resolved Hide resolved

Use Literal for internal_scheme

c730827

Co-authored-by: PietroPasotti <[email protected]>

PietroPasotti mentioned this pull request May 3, 2024

TLS integration for tempo endpoints and server #98

Merged

PietroPasotti suggested changes May 3, 2024

View reviewed changes

lib/charms/tempo_k8s/v2/tracing.py Show resolved Hide resolved

PietroPasotti approved these changes May 3, 2024

View reviewed changes

PietroPasotti merged commit 228848b into main May 3, 2024
13 checks passed

PietroPasotti deleted the traefik-route branch May 3, 2024 14:26

This was referenced May 24, 2024

Support environments outside of K8s for the tracing endpoint returned by tempo-k8s's tracing charm lib #110

Closed

Encountering a Bad Gateway error on Grafana when using tempo as a source #114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add traefik_route to serve tracing endpoints through ingress if it exists #94

Add traefik_route to serve tracing endpoints through ingress if it exists #94

mmkay commented Apr 25, 2024 •

edited

Loading

PietroPasotti left a comment

mmkay commented Apr 30, 2024

PietroPasotti left a comment

PietroPasotti left a comment

PietroPasotti left a comment

PietroPasotti left a comment

PietroPasotti left a comment

Add traefik_route to serve tracing endpoints through ingress if it exists #94

Add traefik_route to serve tracing endpoints through ingress if it exists #94

Conversation

mmkay commented Apr 25, 2024 • edited Loading

Issue

Solution

Context

Testing Instructions

Release Notes

PietroPasotti left a comment

Choose a reason for hiding this comment

mmkay commented Apr 30, 2024

PietroPasotti left a comment

Choose a reason for hiding this comment

PietroPasotti left a comment

Choose a reason for hiding this comment

PietroPasotti left a comment

Choose a reason for hiding this comment

PietroPasotti left a comment

Choose a reason for hiding this comment

PietroPasotti left a comment

Choose a reason for hiding this comment

mmkay commented Apr 25, 2024 •

edited

Loading